imp: allow memo strings instead of keys for transfer authorizations #6268
Conversation
crodriguezvega
requested review from
colin-axner,
AdityaSripal,
charleenfei,
damiannolan,
chatton,
DimitrisJim and
srdtrk
as code owners
WalkthroughThe recent updates refine the authorization mechanism for IBC transfers by specifying JSON-encoded memo strings instead of memo packet data keys. This change enhances security and precision in transfer authorizations, allowing validation of entire memo strings rather than just individual keys or wildcard entries. Changes
Assessment against linked issues
The updates effectively address the core requirements from the linked issue regarding the authorization of specific memo strings, enhancing the security and functionality of the transfer authorization mechanism. However, the consideration of removing blanket authorizations for keys remains unaddressed in this iteration. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
crodriguezvega
added
priority
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
docs/docs/02-apps/01-transfer/08-authorizations.md (3)
Line range hint
21-21: Consider revising to "maximum number of tokens" for grammatical accuracy.- specifies the maximum amount of tokens + specifies the maximum number of tokens
Line range hint
21-21: Use an en dash for numerical ranges for better readability.- i.e. 2^256 - 1 + i.e. 2^256–1
Line range hint
33-33: Remove the repeated word "invalid".- invalid - there are duplicate entries + - there are duplicate entries
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
modules/apps/transfer/types/authz.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (5)
- docs/docs/02-apps/01-transfer/08-authorizations.md (2 hunks)
- modules/apps/transfer/types/keys.go (1 hunks)
- modules/apps/transfer/types/transfer_authorization.go (3 hunks)
- modules/apps/transfer/types/transfer_authorization_test.go (8 hunks)
- proto/ibc/applications/transfer/v1/authz.proto (1 hunks)
Files skipped from review due to trivial changes (1)
- proto/ibc/applications/transfer/v1/authz.proto
Additional Context Used
LanguageTool (3)
docs/docs/02-apps/01-transfer/08-authorizations.md (3)
Near line 21: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...SpendLimitthat specifies the maximum amount of tokens the grantee can transfer. The...
Near line 21: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...r a 256-bit unsigned integer (i.e. 2^256 - 1) is used for the amount, in which case...
Near line 33: Possible typo: you repeated a word
Context: ...nvalid - there are duplicate entries in theAllowList- thememofield is not allowed by `Allowed...
Path-based Instructions (4)
modules/apps/transfer/types/keys.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.docs/docs/02-apps/01-transfer/08-authorizations.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"modules/apps/transfer/types/transfer_authorization.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/types/transfer_authorization_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (8)
modules/apps/transfer/types/keys.go (1)
33-33: Change toAllowAllPacketDataKeysaligns with the new functionality of handling memo strings.Verification successful
The usage of
AllowAllPacketDataKeysacross the codebase appears to be consistent with its intended functionality of handling all memo strings in authorization transfer messages. The wildcard character "*" assigned to it supports this broad permission setting, which aligns with the changes made in the PR.
File:
modules/apps/transfer/types/transfer_authorization.go
- Usage in conditions to check if all packet data is allowed, which is consistent with the wildcard functionality.
File:
modules/apps/transfer/types/keys.go
- Definition and comment align with allowing all memo strings.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `AllowAllPacketDataKeys` to ensure it's correctly implemented with the new memo strings functionality. # Test: Search for the usage of `AllowAllPacketDataKeys`. Expect: Usage consistent with handling memo strings. rg --type go 'AllowAllPacketDataKeys'Length of output: 688
docs/docs/02-apps/01-transfer/08-authorizations.md (1)
25-25: Documentation update correctly reflects the new handling of JSON-encoded memo strings in transfer authorizations.modules/apps/transfer/types/transfer_authorization.go (3)
4-4: Addition of"bytes"import is appropriate for the operations performed invalidateMemo.
135-143: Enhancements toValidateBasicfor JSON parsing and validation of memo strings are correctly implemented.
179-202: Modifications tovalidateMemoto handle different memo string scenarios are well-implemented and robust.modules/apps/transfer/types/transfer_authorization_test.go (3)
17-37: Introduction of constantstestMemo1andtestMemo2for testing different memo scenarios is appropriate.
152-160: Test case "success: AllowedPacketData allows any packet" correctly verifies the handling of wildcard memo strings.
178-182: Test case "success: compact transfer memo allowed" effectively verifies the handling of compact memo strings.
| if len(allocation.AllowedPacketData) > 0 && allocation.AllowedPacketData[0] != AllowAllPacketDataKeys { | ||
| jsonObject := make(map[string]interface{}) | ||
| for _, elem := range allocation.AllowedPacketData { | ||
| err := json.Unmarshal([]byte(elem), &jsonObject) | ||
| if err != nil { | ||
| return errorsmod.Wrapf(ErrInvalidAuthorization, "allowed packet data contains a non JSON-encoded string: %s", elem) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This checks that if the wildcard is not used, then all elements should be JSON-encoded strings.
| @@ -131,6 +131,16 @@ func (a TransferAuthorization) ValidateBasic() error { | |||
| } | |||
| found[allocation.AllowList[i]] = true | |||
| } | |||
|
|
|||
| if len(allocation.AllowedPacketData) > 0 && allocation.AllowedPacketData[0] != AllowAllPacketDataKeys { | |||
There was a problem hiding this comment.
Should we also put a limit on how many items we allow in AllowedPacketData?
| dst := &bytes.Buffer{} | ||
| json.Compact(dst, []byte(allowedMemo)) | ||
| compactAllowedMemo := dst.String() | ||
|
|
||
| keys := make([]string, 0, len(jsonObject)) | ||
| for k := range jsonObject { | ||
| keys = append(keys, k) | ||
| } | ||
| sort.Strings(keys) | ||
| dst = &bytes.Buffer{} | ||
| json.Compact(dst, []byte(memo)) | ||
| compactMemo := dst.String() |
There was a problem hiding this comment.
Maybe this is over engineering it, but this compacts the JSON strings to prevent a mismatch due to empty spaces.
| // allow list of packet data keys, an empty list prohibits all packet data keys; | ||
| // a list only with "*" permits any packet data key | ||
| // allow list of (JSON-encoded) memo strings, an empty list prohibits all memo strings; | ||
| // a list only with "*" permits any memo string | ||
| repeated string allowed_packet_data = 5; |
There was a problem hiding this comment.
Maybe in v9 we can rename this field?
| @@ -30,7 +30,7 @@ const ( | |||
| // DenomPrefix is the prefix used for internal SDK coin representation. | |||
| DenomPrefix = "ibc" | |||
|
|
|||
| // AllowAllPacketDataKeys holds the string key that allows all packet data keys in authz transfer messages | |||
| // AllowAllPacketDataKeys holds the string key that allows all memo strings in authz transfer messages | |||
| AllowAllPacketDataKeys = "*" | |||
There was a problem hiding this comment.
Maybe this constant can also be renamed in v9? We could also add a new constant with a more accurate name for the back ports.
There was a problem hiding this comment.
Yeah, maybe you can rename it on this PR and then change it back in the backport
There was a problem hiding this comment.
I am going to rename it in a followup PR, just to make the backport easier.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
186-186: Remove trailing spaces.- * (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use JSON-encoded memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization. + * (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use JSON-encoded memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization.
Line range hint
171-171: Use a descriptive text for the URL to improve accessibility and SEO.- * [\#6193](https://github.com/cosmos/ibc-go/pull/6193) Bump Cosmos SDK to v0.50.6. + * [Bump Cosmos SDK to v0.50.6](https://github.com/cosmos/ibc-go/pull/6193).
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- modules/apps/transfer/types/transfer_authorization.go (3 hunks)
- modules/apps/transfer/types/transfer_authorization_test.go (8 hunks)
Files not reviewed due to errors (1)
- modules/apps/transfer/types/transfer_authorization_test.go (no review received)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/transfer/types/transfer_authorization.go
Additional Context Used
Path-based Instructions (2)
modules/apps/transfer/types/transfer_authorization_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
…specific-memo-string
| for _, elem := range allocation.AllowedPacketData { | ||
| err := json.Unmarshal([]byte(elem), &jsonObject) | ||
| if err != nil { | ||
| return errorsmod.Wrapf(ErrInvalidAuthorization, "allowed packet data contains a non JSON-encoded string: %s", elem) |
There was a problem hiding this comment.
Why do we require them to be JSON encoded? Can we remove this requirement please and just use any string?
There was a problem hiding this comment.
Sure, I can remove it. Although I thought that, even though it's not enforced, the consensus was that the memo should a JSON-encoded string. Do you know of use cases where that would not be the case?
There was a problem hiding this comment.
Ok, I changed it here. If during the PR review we decide to go back to enforcing JSON, we can just revert that commit.
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
172-172: Remove bare URL and replace with a descriptive link text.- * [\#6193](https://github.com/cosmos/ibc-go/pull/6193) Bump Cosmos SDK to v0.50.6. + * [\#6193](Bump Cosmos SDK to v0.50.6)[https://github.com/cosmos/ibc-go/pull/6193]
Line range hint
187-187: Remove trailing spaces.- * (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization. + * (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
modules/apps/transfer/types/authz.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- docs/docs/02-apps/01-transfer/08-authorizations.md (2 hunks)
- modules/apps/transfer/types/transfer_authorization.go (2 hunks)
- modules/apps/transfer/types/transfer_authorization_test.go (6 hunks)
- proto/ibc/applications/transfer/v1/authz.proto (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/apps/transfer/types/transfer_authorization.go
- proto/ibc/applications/transfer/v1/authz.proto
Additional Context Used
LanguageTool (4)
docs/docs/02-apps/01-transfer/08-authorizations.md (4)
Near line 21: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...SpendLimitthat specifies the maximum amount of tokens the grantee can transfer. The...
Near line 21: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...r a 256-bit unsigned integer (i.e. 2^256 - 1) is used for the amount, in which case...
Near line 25: You might be missing the article “the” here.
Context: ...ent equal to"*", then any content inmemofield will be allowed. Setting a ...
Near line 33: Possible typo: you repeated a word
Context: ...nvalid - there are duplicate entries in theAllowList- thememofield is not allowed by `Allowed...
Path-based Instructions (3)
docs/docs/02-apps/01-transfer/08-authorizations.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"modules/apps/transfer/types/transfer_authorization_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (7)
docs/docs/02-apps/01-transfer/08-authorizations.md (1)
Line range hint
25-55: Ensure the documentation accurately reflects the new functionality ofAllowedPacketData. The description should clearly differentiate between allowing any memo content with"*"and restricting to specific JSON-encoded strings.modules/apps/transfer/types/transfer_authorization_test.go (6)
14-17: Ensure the test memos (testMemo1andtestMemo2) are representative of typical use cases and cover edge cases for JSON-encoded memo strings.
128-128: Verify that the memo used in the test matches the allowed memo strings to ensure the test's validity.Verification successful
The verification process aimed to ensure that the memo used in the test (
testMemo1) matches the allowed memo strings to maintain the test's validity. The script output indicates thattestMemo1is defined and used consistently within the test file, and it is also included in theallowedListarrays, which presumably define the set of valid memo strings for the tests.Here's a breakdown of the findings:
testMemo1is defined with a specific JSON structure.- It is assigned to
msgTransfer.Memoin multiple test scenarios.- It is included in
allowedListin different contexts, which suggests that it is indeed considered a valid memo for the purposes of these tests.Based on this information, it appears that the memo used in the test (
testMemo1) is consistent with the allowed memo strings defined within the test file itself. Therefore, the test's validity in terms of using an appropriate memo string is confirmed.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the memo used in the test matches the allowed memo strings. # Test: Search for the memo usage in the test. Expect: Only occurances of the allowed memo strings. rg --type go $'testMemo1'Length of output: 886
141-143: Ensure that the test covers scenarios where both memos (testMemo1andtestMemo2) are allowed and verify their correct handling in the authorization logic.
158-158: This test case is crucial as it checks the behavior when no memo strings are allowed but a memo is provided. Ensure the error handling is robust and the error message is clear and informative.
167-173: This test case effectively checks the scenario where a non-allowed memo is used. Confirm that the error message accurately reflects the unauthorized memo usage.
316-322: The test case for wildcard allowed packet data is important for ensuring that the authorization logic correctly handles cases where any memo string is permitted. Verify that this functionality is consistent with the intended behavior described in the documentation.
| @@ -22,7 +22,7 @@ It takes: | |||
|
|
|||
| - an `AllowList` list that specifies the list of addresses that are allowed to receive funds. If this list is empty, then all addresses are allowed to receive funds from the `TransferAuthorization`. | |||
|
|
|||
| - an `AllowedPacketData` list that specifies the list of memo packet data keys that are allowed to send the packet. If this list is empty, then only an empty memo is allowed (a `memo` field with non-empty content will be denied). If this list includes a single element equal to `"*"`, then any content in `memo` field will be allowed. | |||
| - an `AllowedPacketData` list that specifies the list of memo strings that are allowed to be included in the memo field of the packet. If this list is empty, then only an empty memo is allowed (a `memo` field with non-empty content will be denied). If this list includes a single element equal to `"*"`, then any content in `memo` field will be allowed. | |||
There was a problem hiding this comment.
Consider adding the article "the" before "memo field" for grammatical correctness.
- then any content in `memo` field will be allowed.
+ then any content in the `memo` field will be allowed.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - an `AllowedPacketData` list that specifies the list of memo strings that are allowed to be included in the memo field of the packet. If this list is empty, then only an empty memo is allowed (a `memo` field with non-empty content will be denied). If this list includes a single element equal to `"*"`, then any content in `memo` field will be allowed. | |
| - an `AllowedPacketData` list that specifies the list of memo strings that are allowed to be included in the memo field of the packet. If this list is empty, then only an empty memo is allowed (a `memo` field with non-empty content will be denied). If this list includes a single element equal to `"*"`, then any content in the `memo` field will be allowed. |
…specific-memo-string
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
173-173: Remove bare URL usage for better readability.- * [\#6193](https://github.com/cosmos/ibc-go/pull/6193) Bump Cosmos SDK to v0.50.6. + * [Bump Cosmos SDK to v0.50.6](https://github.com/cosmos/ibc-go/pull/6193).
Line range hint
188-188: Remove trailing spaces.- * (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization. + * (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
There was a problem hiding this comment.
Should we document that existing authorizations may need to be revoked here since the behaviour is changed?
Nice, thanks for changing this @crodriguezvega!
| @@ -60,6 +60,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able. | |||
| * (core/02-client, core/03-connection, apps/27-interchain-accounts) [\#6256](https://github.com/cosmos/ibc-go/pull/6256) Add length checking of array fields in messages. | |||
| * (apps/27-interchain-accounts, apps/tranfer, apps/29-fee) [\#6253](https://github.com/cosmos/ibc-go/pull/6253) Allow channel handshake to succeed if fee middleware is wired up on one side, but not the other. | |||
| * (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization. | |||
There was a problem hiding this comment.
State machine breaking, right?
| @@ -30,7 +30,7 @@ const ( | |||
| // DenomPrefix is the prefix used for internal SDK coin representation. | |||
| DenomPrefix = "ibc" | |||
|
|
|||
| // AllowAllPacketDataKeys holds the string key that allows all packet data keys in authz transfer messages | |||
| // AllowAllPacketDataKeys holds the string key that allows all memo strings in authz transfer messages | |||
| AllowAllPacketDataKeys = "*" | |||
There was a problem hiding this comment.
Yeah, maybe you can rename it on this PR and then change it back in the backport
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- modules/apps/transfer/types/transfer_authorization.go (3 hunks)
- modules/apps/transfer/types/transfer_authorization_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/transfer/types/transfer_authorization_test.go
Additional Context Used
Path-based Instructions (1)
modules/apps/transfer/types/transfer_authorization.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (3)
modules/apps/transfer/types/transfer_authorization.go (3)
Line range hint
157-187: Consider removingstrings.TrimSpaceif memo strings are pre-sanitized.Verification successful
The investigation into the handling of memo strings in the
validateMemofunction reveals that memo strings are not pre-sanitized before being passed to this function. The direct assignment of memo values in test cases and the absence of any preprocessing steps in the code snippets confirm this. Therefore, the use ofstrings.TrimSpacewithinvalidateMemois necessary to ensure accurate validation against the allowed memos list, addressing the concern raised in the review comment.
- The use of
strings.TrimSpaceis justified to handle potential leading and trailing spaces in memo strings effectively.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if memo strings are pre-sanitized before being passed to `validateMemo`. # Test: Search for the function usage and check if memo strings are pre-sanitized. rg --type go $'validateMemo'Length of output: 447
Script:
#!/bin/bash # Description: Check for the definition and handling of `msgTransfer` to determine if `Memo` is pre-sanitized. # Search for the definition and usage of `msgTransfer` and any operations on `Memo`. rg --type go "msgTransfer" -A 10Length of output: 22107
Line range hint
157-187: LGTM! Good use of gas metering to prevent abuse inisAllowedAddress.
Line range hint
157-187: LGTM! Comprehensive checks inValidateBasic.
| _, ok := jsonObject[key] | ||
| if ok { | ||
| delete(jsonObject, key) | ||
| if strings.TrimSpace(memo) == strings.TrimSpace(allowedMemo) { |
There was a problem hiding this comment.
Thinking more about this, I think trim spaces doesn't make much sense in here. We are allowing a specific memo string, and not making any assumptions about the format such as it being json. It makes no sense that if a user allowed "rent payment", then they also allow "rentpayment". Especially if these transactions would be indexed by an indexer later based on the memo.
Maybe worth asking others, but I'm starting to feel more strongly about this.
There was a problem hiding this comment.
TrimSpace only trims leading and trailing spaces, not spaces in between the text. That's why I thought that it would make sense to use it.
There was a problem hiding this comment.
Ah ok, I'm back to being indifferent about it then. Still ran be removed imo. We shouldn't be cleaning up user mess XD
Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>
Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/apps/transfer/types/transfer_authorization.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/transfer/types/transfer_authorization.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/apps/transfer/types/transfer_authorization.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/transfer/types/transfer_authorization.go
…specific-memo-string
…specific-memo-string
|
crodriguezvega
deleted the
carlos/6095-allow-ics20-authorisation-for-a-specific-memo-string
branch
…6268) * imp: allow memo strings instead of keys for transfer authorizations * add changelog * handle error from compact * return error * improve test * not enforce that memo strings of allowed packet data must be JSON-encoded strings * use slices contains to check if memo is allowed * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * lint --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> (cherry picked from commit 0a22b7a) # Conflicts: # docs/docs/02-apps/01-transfer/08-authorizations.md # modules/apps/transfer/types/authz.pb.go # modules/apps/transfer/types/keys.go # modules/apps/transfer/types/transfer_authorization.go # modules/apps/transfer/types/transfer_authorization_test.go # proto/ibc/applications/transfer/v1/authz.proto
…6268) * imp: allow memo strings instead of keys for transfer authorizations * add changelog * handle error from compact * return error * improve test * not enforce that memo strings of allowed packet data must be JSON-encoded strings * use slices contains to check if memo is allowed * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * lint --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> (cherry picked from commit 0a22b7a) # Conflicts: # CHANGELOG.md # docs/docs/02-apps/01-transfer/08-authorizations.md # modules/apps/transfer/types/transfer_authorization.go
…backport #6268) (#6289) * imp: allow memo strings instead of keys for transfer authorizations (#6268) * imp: allow memo strings instead of keys for transfer authorizations * add changelog * handle error from compact * return error * improve test * not enforce that memo strings of allowed packet data must be JSON-encoded strings * use slices contains to check if memo is allowed * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * lint --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> (cherry picked from commit 0a22b7a) # Conflicts: # CHANGELOG.md # docs/docs/02-apps/01-transfer/08-authorizations.md # modules/apps/transfer/types/transfer_authorization.go * fix conflicts * can't use slices import * Revert "can't use slices import" This reverts commit e9d9f69. * remove docs --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
…backport #6268) (#6288) * imp: allow memo strings instead of keys for transfer authorizations (#6268) * imp: allow memo strings instead of keys for transfer authorizations * add changelog * handle error from compact * return error * improve test * not enforce that memo strings of allowed packet data must be JSON-encoded strings * use slices contains to check if memo is allowed * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * lint --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> (cherry picked from commit 0a22b7a) # Conflicts: # docs/docs/02-apps/01-transfer/08-authorizations.md # modules/apps/transfer/types/authz.pb.go # modules/apps/transfer/types/keys.go # modules/apps/transfer/types/transfer_authorization.go # modules/apps/transfer/types/transfer_authorization_test.go # proto/ibc/applications/transfer/v1/authz.proto * fix conflicts * don't use slices * use sdkerrors * remove docs --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Description
closes: #6095
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
Documentation
AllowedPacketDatain theTransferAuthorizationdocumentation to specify JSON-encoded memo strings.New Features
Refactor
validateMemofunction, enhancing memo validation efficiency and clarity.